Skip to content

feat(cct-sdk): Add transfer pool ownership op solana - #358

Merged
mervin-link merged 4 commits into
cct-sdkfrom
feat/DAPP-11130-transfer-pool-ownership
Aug 21, 2026
Merged

feat(cct-sdk): Add transfer pool ownership op solana#358
mervin-link merged 4 commits into
cct-sdkfrom
feat/DAPP-11130-transfer-pool-ownership

Conversation

@mervin-link

@mervin-link mervin-link commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

What

  • DAPP-11130
  • Add Solana transferPoolOwnership and unsigned transaction generation to the CCT SDK facade
  • Validate proposed owners against the current pool owner and default public key

Why

  • Enable canonical Solana token pool ownership transfer workflows

@mervin-link
mervin-link requested a review from apedrob August 17, 2026 09:26
@mervin-link
mervin-link requested review from a team, PabloMansanet and aelmanaa as code owners August 17, 2026 09:26
@vercel

vercel Bot commented Aug 17, 2026

Copy link
Copy Markdown

You must have Developer access to commit code to Chainlink Labs on Vercel. If you contact an administrator and receive Developer access, commit again to see your changes.

Learn more: https://vercel.com/docs/accounts/team-members-and-roles/access-roles#team-level-roles

@aelmanaa

aelmanaa commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

buildUnsigned() calls GetTokenPoolState().query() which throws CCIPTokenPoolStateNotFoundError when the pool account is missing — this occurs on both generate and execute paths, before simulation.

Issue:

  • Line 909: generateUnsignedTransferPoolOwnership documents only CCTParamsInvalidError → the not-found error is undocumented.
  • Line 941: transferPoolOwnership documents "If the pool does not exist" under CCTTxFailedErrorfactually wrong. Pool-not-found throws CCIPTokenPoolStateNotFoundError at the state read, not during simulation. Consumers branching on catch (e) { if (e instanceof CCTTxFailedError) } would miss the real error.

Fix: Add @throws {@link CCIPTokenPoolStateNotFoundError} If the token pool account does not exist. to both methods. Drop "the pool does not exist" from transferPoolOwnership's CCTTxFailedError clause.

Proof: Test at 39983bd confirms generateUnsignedTransferPoolOwnership with getAccountInfo returning null throws CCIPTokenPoolStateNotFoundError, not CCTParamsInvalidError/CCTTxFailedError.

@aelmanaa

aelmanaa commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator
  • EVM sibling: transferOwnership / generateUnsignedTransferOwnership (cct/evm/token-admin-registry/operations/transfer-ownership.ts)
  • Solana admin ops: transferAdmin (no Pool infix), mirrored in setRateLimitAdmin, setChainRateLimit

A family-agnostic consumer hits cct.transferOwnership() on EVM but cct.transferPoolOwnership() on Solana — identical concept, inconsistent naming. This is a public-API commitment (auto-exported), so cheapest to fix now before general availability.

Fix: Rename to transferOwnership / generateUnsignedTransferOwnership, and update the internal name = 'transferOwnership' in the operation class. Align proposedOwner param to newOwner for consistency with EVM.

Consistency check: https://github.com/smartcontractkit/ccip-tools-ts/blob/cct-sdk/ccip-sdk/src/cct/evm/token-pool/operations/transfer-ownership.ts\#L21

@aelmanaa

aelmanaa commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Parameter proposedOwner (line 26 in transfer-pool-ownership.ts) vs. EVM's newOwner / Solana's transferAdmin's newAdmin.

  • Defensible: Tracks the on-chain Anchor arg proposed_owner (burnmint-token-pool/src/lib.rs:250)
  • Consistency: If addressing rename op to match EVM, align param to newOwner for consistency with EVM's TransferOwnership operation

Judgment call: Pick one axis (on-chain naming or cross-family naming) and apply to both operation and parameter.

@aelmanaa

aelmanaa commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Line 102 in transfer-pool-ownership.ts: the self-transfer guard throws CCTParamsInvalidError('transferPoolOwnership', ...) with a hardcoded string.

If the operation's name is renamed , this will drift silently.

Fix: Use this.name instead:

if (opts.proposedOwner.equals(new PublicKey(config.owner))) {
  throw new CCTParamsInvalidError(
    this.name,
    'proposedOwner',
    'must not be the current pool owner',
  )
}

(See line 60 in the same file for the parse() guard pattern.)

@aelmanaa

aelmanaa commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary: APPROVE-with-changes

Green in isolation: tsc --noEmit 0 real errors; op test 7/7 passing; type-clean (no any/unsafe-as).

On-chain grounded burnmint-token-pool:

  • Instruction transferOwnership(proposedOwner) + accountsStrict({ state, mint, authority }) matches program constraints
  • Zero-address + self-transfer guards mirror program's require(proposed_owner != owner && != default)

Housekeeping (not a finding)

⚠️ Branch is now behind cct-sdkcct-sdk advanced via main-integration merge 9d6a043 since this branch was created.

Action needed before merge:

  1. Rebase onto current cct-sdk so CI runs on live base
  2. Note: main-integration brought canonical Move-address / format-safe byte codec change; no conflicts expected in Solana files, but re-run test suite after update.

Non-issues (reviewed, not findings)

  • Accept half missing: Two-step can't be completed via SDK, but this is symmetric with EVM (no acceptOwnership there either). In-code TODOs mark follow-up. Recommend tracked ticket for v1.12.
  • Extra validation: Zero-address + self-transfer guards beyond minimum; justified (mirrored on-chain), EVM omits them (OZ Ownable2Step permits those values — genuine chain difference).
  • buildUnsigned re-read on signed path: One extra RPC matches sibling convention (SetRateLimitAdmin, etc.).

@mervin-link

Copy link
Copy Markdown
Collaborator Author

Summary: APPROVE-with-changes

Green in isolation: tsc --noEmit 0 real errors; op test 7/7 passing; type-clean (no any/unsafe-as).

On-chain grounded burnmint-token-pool:

  • Instruction transferOwnership(proposedOwner) + accountsStrict({ state, mint, authority }) matches program constraints
  • Zero-address + self-transfer guards mirror program's require(proposed_owner != owner && != default)

Housekeeping (not a finding)

⚠️ Branch is now behind cct-sdkcct-sdk advanced via main-integration merge 9d6a043 since this branch was created.

Action needed before merge:

  1. Rebase onto current cct-sdk so CI runs on live base
  2. Note: main-integration brought canonical Move-address / format-safe byte codec change; no conflicts expected in Solana files, but re-run test suite after update.

Non-issues (reviewed, not findings)

  • Accept half missing: Two-step can't be completed via SDK, but this is symmetric with EVM (no acceptOwnership there either). In-code TODOs mark follow-up. Recommend tracked ticket for v1.12.
  • Extra validation: Zero-address + self-transfer guards beyond minimum; justified (mirrored on-chain), EVM omits them (OZ Ownable2Step permits those values — genuine chain difference).
  • buildUnsigned re-read on signed path: One extra RPC matches sibling convention (SetRateLimitAdmin, etc.).

@aelmanaa Addressed all your comments.

feat: add accept pool ownership op solana
@mervin-link
mervin-link merged commit 4fd94b1 into cct-sdk Aug 21, 2026
2 of 4 checks passed
@mervin-link
mervin-link deleted the feat/DAPP-11130-transfer-pool-ownership branch August 21, 2026 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants